Vitest: Reset playwright cursor position to avoid hover bug#34765
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Vitest plugin command ChangesMouse Position Reset Integration
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Setup as resetMousePositionBeforeTests
participant BrowserCommands as vitest/browser.commands
participant PlaywrightPage as Playwright.page
TestRunner->>Setup: beforeEach()
Setup->>BrowserCommands: import commands
alt commands.resetMousePosition is function
Setup->>BrowserCommands: await commands.resetMousePosition()
BrowserCommands->>PlaywrightPage: page.mouse.move(-1000, -1000)
else module missing
Setup->>`@vitest/browser/context.commands`: import commands (fallback)
alt fallback.commands.resetMousePosition is function
Setup->>`@vitest/browser/context.commands`: await commands.resetMousePosition()
`@vitest/browser/context.commands-`>>PlaywrightPage: page.mouse.move(-1000, -1000)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/vitest/templates/vitest.config.template.ts (1)
12-17: ⚡ Quick winUse a zero-argument
BrowserCommandsignature here.Line 12 declares
BrowserCommand<[number, number]>, but the command implementation takes onlyctxand is called with no arguments (seecommands.resetMousePosition()in the setup-file). The generic should beBrowserCommand<[]>to match the actual payload.♻️ Proposed fix
-const resetMousePosition: BrowserCommand<[number, number]> = async (ctx) => { +const resetMousePosition: BrowserCommand<[]> = async (ctx) => { if (ctx.provider.name !== 'playwright') throw new Error('resetMousePosition requires the Playwright provider'); const frame = await ctx.frame(); await frame.page().mouse.move(-1000, -1000); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/vitest/templates/vitest.config.template.ts` around lines 12 - 17, The BrowserCommand generic for resetMousePosition is incorrect: change its type from BrowserCommand<[number, number]> to BrowserCommand<[]> so the command signature matches its zero-argument implementation (resetMousePosition(ctx) is invoked with no payload). Update the declaration of resetMousePosition to use BrowserCommand<[]> and ensure any references or exports that rely on its type reflect the new generic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/addons/vitest/templates/vitest.config.template.ts`:
- Around line 12-17: The BrowserCommand generic for resetMousePosition is
incorrect: change its type from BrowserCommand<[number, number]> to
BrowserCommand<[]> so the command signature matches its zero-argument
implementation (resetMousePosition(ctx) is invoked with no payload). Update the
declaration of resetMousePosition to use BrowserCommand<[]> and ensure any
references or exports that rely on its type reflect the new generic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b2d4a15-b49f-4dd5-9901-25328eb6b62a
📒 Files selected for processing (4)
code/addons/vitest/src/vitest-plugin/setup-file.tscode/addons/vitest/templates/vitest.config.3.2.template.tscode/addons/vitest/templates/vitest.config.4.template.tscode/addons/vitest/templates/vitest.config.template.ts
There was a problem hiding this comment.
Pull request overview
This PR aims to make Vitest Browser (Playwright) runs more reliable on Chromium/Linux by forcing the initial mouse cursor position away from (0,0), preventing unintended :hover recalculations that can skew axe-core a11y results.
Changes:
- Added a
resetMousePositionPlaywright browser command to Vitest config templates. - Invoked
commands.resetMousePosition()from the addon’s internal Vitest setup file in abeforeEachhook.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| code/addons/vitest/templates/vitest.config.template.ts | Adds a Playwright browser command and registers it in test.browser.commands. |
| code/addons/vitest/templates/vitest.config.4.template.ts | Same as above for the Vitest 4 template variant. |
| code/addons/vitest/templates/vitest.config.3.2.template.ts | Same as above for the Vitest 3.2 template variant. |
| code/addons/vitest/src/vitest-plugin/setup-file.ts | Adds a beforeEach hook that conditionally calls the new browser command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
53fcb3e to
ab58c52
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/vitest/src/vitest-plugin/setup-file.ts (1)
40-53: ⚡ Quick winRemove unnecessary error handling and simplify the defensive check
Line 41's
inoperator is safe. Thecommandsexport is guaranteed to exist in both Vitest 3.x and 4.x, so the destructuring on Line 40 will always succeed. The catch block checking for"Cannot destructure property 'commands'"is dead code and can be removed. Simplify to safely check for theresetMousePositionmethod:Proposed fix
export const resetMousePositionBeforeTests = async () => { try { const { commands } = await import('vitest/browser'); - if ('resetMousePosition' in commands && isFunction(commands.resetMousePosition)) { + if (isFunction(commands.resetMousePosition)) { await commands.resetMousePosition(); } } catch (error) { // Ignore when vitest/browser is not found (Vitest 3) if (error instanceof Error && error.message.includes("Cannot find module 'vitest/browser'")) { return; } - // Ignore when commands is not exported by vitest/browser (Vitest 3) - if ( - error instanceof Error && - error.message.includes("Cannot destructure property 'commands'") - ) { - return; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/vitest/src/vitest-plugin/setup-file.ts` around lines 40 - 53, Remove the dead error check that looks for the "Cannot destructure property 'commands'" message in the catch block: keep the current dynamic import of 'vitest/browser' and the defensive runtime check for commands.resetMousePosition (the in operator and isFunction(commands.resetMousePosition)), but delete the entire conditional that tests error.message.includes("Cannot destructure property 'commands'") so the catch only ignores missing-module errors and otherwise rethrows or handles real errors; target the try/catch surrounding the import and the symbols commands and resetMousePosition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/addons/vitest/src/vitest-plugin/setup-file.ts`:
- Around line 40-53: Remove the dead error check that looks for the "Cannot
destructure property 'commands'" message in the catch block: keep the current
dynamic import of 'vitest/browser' and the defensive runtime check for
commands.resetMousePosition (the in operator and
isFunction(commands.resetMousePosition)), but delete the entire conditional that
tests error.message.includes("Cannot destructure property 'commands'") so the
catch only ignores missing-module errors and otherwise rethrows or handles real
errors; target the try/catch surrounding the import and the symbols commands and
resetMousePosition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12bbd42f-ce37-4159-ba2b-0b26502e97db
📒 Files selected for processing (2)
code/addons/vitest/src/vitest-plugin/setup-file.test.tscode/addons/vitest/src/vitest-plugin/setup-file.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/vitest/src/vitest-plugin/setup-file.test.ts
AriPerkkio
left a comment
There was a problem hiding this comment.
Looks good!
Test cases mock too much but that's out of scope of this PR. 🥹
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/vitest/src/vitest-plugin/setup-file.test.ts`:
- Line 119: The test currently uses a broad await
expect(resetMousePositionBeforeTests()).rejects.toThrow(); update it to assert
the specific error or message thrown by resetMousePositionBeforeTests() to avoid
false positives — for example, change the assertion to await
expect(resetMousePositionBeforeTests()).rejects.toThrowError(<expected message
or regex>) or toThrowError(<ExpectedErrorClass>) and, if needed, adjust the test
stub/mocked dependency that causes the throw so the expected message/class is
known; target the resetMousePositionBeforeTests() call when making this change.
- Around line 82-138: Add a brief inline comment in the test suite above the
per-test vi.doMock() usages explaining that vi.doMock() is intentionally used
per-test as an exception to the spy-mocking rules because the implementation
under test uses dynamic await import() with error handling and needs per-case
module resolution/behavior (reference the vi.doMock calls and the
resetMousePositionBeforeTests tests); mention that hoisted vi.mock(spy: true)
cannot express these per-test variations, and suggest documenting or opening a
discussion in the spy-mocking guidelines if desired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2861e223-dba4-4fd7-acea-894d018c1b54
📒 Files selected for processing (2)
code/addons/vitest/src/vitest-plugin/setup-file.test.tscode/addons/vitest/src/vitest-plugin/setup-file.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/vitest/src/vitest-plugin/setup-file.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
296d0fc to
e1a8477
Compare
e1a8477 to
069a410
Compare
See internal bug report.
What I did
beforeEachto force the initial cursor position away from 0, 0This should address a bug on Chromium in Linux where the
:hoverstate gets recomputed without any actual mouse input events, and applied to elements at 0, 0. The fact that:hovergets applied causesaxe-coreresults to be unreliable, as the default state of a component is no longer tested.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
I'm open to suggestions here.
Manual testing
git clone git@github.com:Sidnioulz/mealdrop.gitcd mealdrop && git checkout tree/telemetry-qa-3yarn && yarn storybookDocumentation
@jonniebigodes @yannbf do you think we need to document this in troubleshooting for the Vitest addon? Or should we add a JSDoc explanation for the browser command in the templates?
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-34765-sha-ab58c528. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34765-sha-ab58c528 sandboxor in an existing project withnpx storybook@0.0.0-pr-34765-sha-ab58c528 upgrade.More information
0.0.0-pr-34765-sha-ab58c528sidnioulz/reset-vitest-cursor-positionab58c5281778511056)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34765Summary by CodeRabbit
Bug Fixes
Tests